feat: add Twilio Lookup v2 API tests#1285
Merged
Merged
Conversation
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBumps dependency pins and replace directives, adds wait-criterion helpers, sets ToolchainConfig phoneLookupMode to 'disabled' in test manifest, and introduces an e2e TestPhoneLookupMode with subtests for disabled, log, and enabled behaviors. ChangesPhone Lookup Mode E2E
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant Config as ToolchainConfig
participant Signup as UserSignup
participant Endpoint as /api/v1/signup/verification
Test->>Config: Set or verify phoneLookupMode (disabled/log/enabled)
Test->>Signup: Create UserSignup with phone number
Test->>Endpoint: PUT /api/v1/signup/verification
Endpoint->>Signup: Evaluate phone lookup and update annotations
Endpoint->>Test: Return HTTP status and verification code (if any)
Test->>Signup: Assert annotations and response match expected mode behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Comment |
coderabbitai
Bot
added
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/parallel/phone_lookup_test.go (1)
128-132: 💤 Low valueConsider simplifying the phone number generation logic.
The current implementation works correctly but the slice operation
suffix[len(suffix)-5:]could be more direct. Consider taking the first 5 digits after generating the UUID string:func uniqueUKPhoneNumber() string { - // UK mobile numbers are 10 digits after country code; use a unique suffix to avoid phone-in-use conflicts - suffix := strings.ReplaceAll(uuid.Must(uuid.NewV4()).String(), "-", "")[:10] - return "77009" + suffix[len(suffix)-5:] + // UK mobile numbers are 10 digits after country code; use a unique 5-digit suffix to avoid phone-in-use conflicts + suffix := strings.ReplaceAll(uuid.Must(uuid.NewV4()).String(), "-", "")[:5] + return "77009" + suffix }This eliminates the need to slice from the end and makes the intent clearer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/parallel/phone_lookup_test.go` around lines 128 - 132, The uniqueUKPhoneNumber function uses a UUID-derived suffix and then takes the last 5 chars via suffix[len(suffix)-5:], which is unnecessarily indirect; change it to take the first 5 characters of the generated suffix (e.g., suffix[:5]) and return "77009" + that substring so the intent is clearer and simpler while preserving the unique phone number format.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Around line 145-147: Remove the temporary forked-module replace directives
from go.mod before merging: delete the two replace lines that map
github.com/codeready-toolchain/api => github.com/fbm3307/toolchainapi@... and
github.com/codeready-toolchain/toolchain-common =>
github.com/fbm3307/toolchain-common@.... If those forks are required while
upstream PRs are pending, add a clear TODO comment above each replace with the
upstream PR URLs and a plan to remove them, but ensure the final merge removes
the replace mappings so builds use the canonical github.com/codeready-toolchain
modules.
---
Nitpick comments:
In `@test/e2e/parallel/phone_lookup_test.go`:
- Around line 128-132: The uniqueUKPhoneNumber function uses a UUID-derived
suffix and then takes the last 5 chars via suffix[len(suffix)-5:], which is
unnecessarily indirect; change it to take the first 5 characters of the
generated suffix (e.g., suffix[:5]) and return "77009" + that substring so the
intent is clearer and simpler while preserving the unique phone number format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: df94b267-4f98-4cff-a556-b6e27331a59d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
deploy/host-operator/e2e-tests/toolchainconfig.yamlgo.modtest/e2e/parallel/phone_lookup_test.gotestsupport/wait/host.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: GolangCI Lint
- GitHub Check: govulncheck
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
deploy/host-operator/e2e-tests/toolchainconfig.yamlgo.modtestsupport/wait/host.gotest/e2e/parallel/phone_lookup_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-01T09:54:02.406Z
Learnt from: fbm3307
Repo: codeready-toolchain/toolchain-e2e PR: 1175
File: deploy/base1ns-gotemplate/cluster.yaml:69-70
Timestamp: 2025-08-01T09:54:02.406Z
Learning: The codeready-toolchain/toolchain-e2e project is not using the latest version of Kubernetes yet, so deprecated API groups like `ingresses.extensions` may still be functional in their current environment. Consider this when reviewing resource quotas and API usage.
Applied to files:
go.mod
🔇 Additional comments (4)
testsupport/wait/host.go (2)
488-514: LGTM!
1729-1740: LGTM!deploy/host-operator/e2e-tests/toolchainconfig.yaml (1)
23-23: LGTM!test/e2e/parallel/phone_lookup_test.go (1)
18-126: LGTM!
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
removed
dependencies
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
added
dependencies
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
removed
dependencies
metlos
reviewed
Jun 4, 2026
Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
coderabbitai
Bot
added
dependencies
coderabbitai
Bot
added
the
test
- Resolve go.mod/go.sum conflicts from upstream merge - Update replace directives to latest toolchainapi and toolchain-common - Update phone_lookup_test.go to use consolidated phone-lookup-details JSON annotation instead of removed individual annotation constants - Use PhoneLookupExcludedCountries config in US number skip test Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
…ommon master Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Comment on lines
+49
to
+53
| t.Run("phoneLookupMode can be set on ToolchainConfig", func(t *testing.T) { | ||
| // Non-default value: CRD default "log" is omitted in spec when set explicitly, so use "enabled" to verify persistence. | ||
| hostAwait.UpdateToolchainConfig(t, testconfig.RegistrationService().Verification().PhoneLookupMode(toolchainv1alpha1.PhoneLookupModeEnabled)) | ||
| VerifyToolchainConfig(t, hostAwait, wait.UntilToolchainConfigHasPhoneLookupMode(toolchainv1alpha1.PhoneLookupModeEnabled)) | ||
| }) |
Collaborator
There was a problem hiding this comment.
Do we really need to have a test for this? Let's keep the e2e tests focused on the functionality
Comment on lines
+70
to
+86
| t.Run("log mode stores phone lookup annotations when lookup succeeds", func(t *testing.T) { | ||
| hostAwait.UpdateToolchainConfig(t, testconfig.RegistrationService().Verification().PhoneLookupMode(toolchainv1alpha1.PhoneLookupModeLog)) | ||
|
|
||
| userSignup, token := signup(t, hostAwait) | ||
| NewHTTPRequest(t). | ||
| InvokeEndpoint("PUT", route+"/api/v1/signup/verification", token, | ||
| fmt.Sprintf(`{ "country_code":"+44", "phone_number":"%s" }`, twilioMagicPhoneHighRiskBlocked), http.StatusNoContent) | ||
|
|
||
| userSignup, err := hostAwait.WaitForUserSignup(t, userSignup.Name, | ||
| wait.UntilUserSignupHasAnnotationNotEmpty(toolchainv1alpha1.UserSignupVerificationCodeAnnotationKey)) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "rejected", phoneLookupDetailsResult(t, userSignup)) | ||
| assert.Equal(t, "high", userSignup.Annotations[toolchainv1alpha1.UserSignupPhoneLookupCarrierRiskAnnotationKey]) | ||
| assert.Equal(t, "true", userSignup.Annotations[toolchainv1alpha1.UserSignupPhoneLookupNumberBlockedAnnotationKey]) | ||
| assert.NotEmpty(t, userSignup.Annotations[toolchainv1alpha1.UserSignupVerificationCodeAnnotationKey]) | ||
| }) |
Collaborator
There was a problem hiding this comment.
The important thing here is also that the UserSignup is not rejected and thus also provisioned - I'm missing the verification here
Contributor
Author
There was a problem hiding this comment.
Added assert.False(t, states.Rejected(userSignup), "UserSignup should NOT be rejected in log mode") to verify the signup is not rejected and can proceed to provisioning.
Collaborator
There was a problem hiding this comment.
why not using VerifyResourcesProvisionedForSignup?
Contributor
Author
There was a problem hiding this comment.
there was no particular reason, we are checking that now
- Remove config persistence and disabled mode tests (duplicative) - Use states.SetRejected/Rejected instead of JSON annotation - Add // given // when // then comments to all subtests - Verify UserSignup is NOT rejected in log mode - Fix US number test: use magic number instead of UUID hex digits - Remove unused phoneLookupDetailsResult helper Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
The previous UUID-derived number had hex chars, then the magic number +11234567890 had invalid area code 123. Use +12025550123 which is in the NANPA 555-0100..0199 range reserved for fictional use, safe with Twilio test credentials. Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…ning - Add t.Parallel() for parallel test execution - Add "enabled mode rejects high-risk phone number" test case - Complete verification flow and call VerifyResourcesProvisionedForSignup in log mode and US-skip tests to confirm full provisioning - Verify states.Rejected assertions in all applicable tests Signed-off-by: Feny Mehta <fbm3307@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
MatousJobanek
approved these changes
Jun 11, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbm3307, MatousJobanek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add twilio v2 lookup api and test related to it .
related Prs:
Registration-Service: codeready-toolchain/registration-service#595
toolchain-Common: codeready-toolchain/toolchain-common#532
host operator: codeready-toolchain/host-operator#1268
API: codeready-toolchain/api#509
Summary by CodeRabbit
New Features
Tests